Skip to content

Feature/ Fetch ENS expiry data for the selected account and add additional transfer ENS validation#2507

Open
PetromirDev wants to merge 1 commit into
ens-reverse-lookup-privacyfrom
feature/ens-expiry-ux
Open

Feature/ Fetch ENS expiry data for the selected account and add additional transfer ENS validation#2507
PetromirDev wants to merge 1 commit into
ens-reverse-lookup-privacyfrom
feature/ens-expiry-ux

Conversation

@PetromirDev

@PetromirDev PetromirDev commented Jun 24, 2026

Copy link
Copy Markdown
Member

To be done:

  • Possibly add an affiliate address (cc @Ivshti so we don't forget)

Implements https://github.com/AmbireTech/ambire-app/issues/6853#issuecomment-4478478774

Changes:

  • The obvious in domains/the ens service/transfer to implement the comment
  • sentToHistory in the activity controller. We need it anyway for ENS so I figured we can also use it instead of traversing every account op
  • recipientDomain to Call - I tested many different solutions in the past two days and trust me, this one is the most reliable because there are many edge cases - the user sends to an address that has ENS, but hasn't typed the ENS; the user sends to an ENS, but then starts a new transfer before the txn is confirmed; the user sends to an ENS, but then adds to batch. We MUST save the ENS to storage ONLY if the user has typed it in manually, the ENS resolves to a different address and the user has typed THE SAME name again. (e.g., yesterday nick.eth -> 1; today nick.eth -> 2) as it would be confusing if the user has never sent to the user by typing in the ENS and we warn him that it changed

Relevant info and docs:

Useful apps:

How to test:

  1. Dashboard banner - open https://ens.tools/find/expiry, pick an ENS, import it and you will see it
  2. Transfer warning. Use the following script, reload the extension and then input the name in the send form. It's important that you do so manually and don't pick the account from the address dropdown as that will fill the field with the address and not display the warning (as it's not needed)
const ENS_NAME    = 'vitalik.eth'
const OLD_ADDRESS = '0x000000000000000000000000000000000000dEaD' 

;(async () => {
  const KEY = 'sentToHistory'
  const raw = (await chrome.storage.local.get(KEY))[KEY]
  const history = raw ? JSON.parse(raw) : { domains: {}, recipients: {} }
  history.domains ??= {}

  history.domains[ENS_NAME.toLowerCase().trim()] = {
    address: OLD_ADDRESS,
    sentAt: Date.now()
  }

  await chrome.storage.local.set({ [KEY]: JSON.stringify(history) })
  console.log('Seeded sentToHistory.domains:', ENS_NAME, history.domains[ENS_NAME.toLowerCase().trim()])
})()

@PetromirDev PetromirDev self-assigned this Jun 24, 2026
@PetromirDev PetromirDev marked this pull request as ready for review June 24, 2026 15:12
@PetromirDev

Copy link
Copy Markdown
Member Author

/review

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🔒 No security concerns identified
📝 TODO sections

⚡ Recommended focus areas for review

Partial storage update leaves state inconsistent

addAccountOp now persists accountsOps and then sentToHistory in two sequential storage.set calls without atomicity or rollback. If the second write fails (or the process crashes between them), the next load will contain the account operation but no corresponding sent-to history. This breaks the fast-path in hasAccountOpsSentTo and can cause false first-send warnings (including address-poisoning checks) for addresses the user has already sent to.

await this.#storage.set('accountsOps', this.#accountsOps)
await this.#storage.set('sentToHistory', this.#sentToHistory)
Public method propagates storage errors

dismissEnsExpiryBannerForTheSelectedAccount awaits this.#storage.set() without a try/catch or withStatus wrapper. If the storage write throws, the error propagates uncaught to the caller, violating the invariant that public controller methods must never propagate errors silently.

async dismissEnsExpiryBannerForTheSelectedAccount() {
  if (!this.account) return

  const expiry = this.#domains?.domains[getAddress(this.account.addr)]?.ensExpiry
  if (!expiry) return

  // Key the dismissal by expiry timestamp so a renewed name (new expiry) shows the banner again.
  const dismissKey = `${this.account.addr}-${expiry.expiresAt}`

  if (!this.dismissedBannerIds[ensExpiryBannerId]) this.dismissedBannerIds[ensExpiryBannerId] = []
  if (!this.dismissedBannerIds[ensExpiryBannerId]!.includes(dismissKey))
    this.dismissedBannerIds[ensExpiryBannerId]!.push(dismissKey)

  await this.#storage.set('selectedAccountDismissedBannerIds', this.dismissedBannerIds)
  this.emitUpdate()
}
Redundant ENS expiry fetches for non-eth names

shouldRefetchEnsExpiry returns true when ensExpiry is null, which is the cached "fetched but not relevant" state for non-.eth names (e.g., DNS names or unregistered names). This causes getEnsExpiry to be invoked again on every selected-account refresh, wasting RPC calls and degrading performance.

export const shouldRefetchEnsExpiry = (entry?: Domains[string]): boolean => {
  const cached = entry?.ensExpiry
  if (!cached) return true

  const isCloseToDeadline = cached.gracePeriodEndsAt - Date.now() < ENS_EXPIRY_WARN_WINDOW_IN_MS
  if (!isCloseToDeadline) return false

  return cached.updatedAt + PERSIST_EXPIRY_FOR_IF_CLOSE_TO_DEADLINE_IN_MS < Date.now()
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant